Skip to content

Conversation

@gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented Oct 16, 2024

Now we only show this warning on l, ll and any command that triggers a stack print.

Lib/pdb.py Outdated
try:
filename = self.curframe.f_code.co_filename
mtime = os.path.getmtime(filename)
self._file_mtime_table.setdefault(filename, mtime)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to overwrite a previous value if the file changed on disk?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, then let's avoid going to the disk when the dict already has a value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we separated the update and check. We do not generate warnings on every update. We want to avoid file change -> update(updated the new info) -> check(fail to find the change).

But yes, we should avoid updating it if not necessary.

Lib/pdb.py Outdated
except KeyboardInterrupt:
self.message('--KeyboardInterrupt--')

def _update_file_mtime(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name seems wrong - it's not "updating" the mtime if it's already there. It only sets it if it's not there.

Also, the docstring of _validate_file_mtime is no longer correct - it's not "since last time we saw it".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to init, do you think it's better? Also changed the docstring a bit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe "save"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "save" better than "init"? Because I think save is similar to update. In this specific case, we only care for the entries that are not intialized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

init sounds to me like you are initialising the actual file_mtime, rather than saving a copy of it. How about save_initial_file_mtime?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay that makes sense. I thought about it and I don't think we need to put it in the command path now. We used to do that because we want the error message on every command. Now we can do it once in setup for all the frames and leave onecmd alone.

Co-authored-by: Irit Katriel <[email protected]>
Co-authored-by: Irit Katriel <[email protected]>
@gaogaotiantian gaogaotiantian merged commit 77cebb1 into python:main Oct 18, 2024
34 checks passed
@gaogaotiantian gaogaotiantian deleted the reduce-file-change-warning branch October 18, 2024 00:29
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants